[AMD] dsv4-fp4-mi355x-atom: latest atom-dev image + opt recipe / DSv4-Pro FP4 MI355X ATOM：升级最新 atom-dev 镜像 + 优化配方#2327
[AMD] dsv4-fp4-mi355x-atom: latest atom-dev image + opt recipe / DSv4-Pro FP4 MI355X ATOM：升级最新 atom-dev 镜像 + 优化配方#2327zufayu wants to merge 7 commits into
Conversation
- image: nightly_202606161823 -> nightly_202607231538 (latest atom-dev) - recipe (dsv4_fp4_mi355x_atom.sh): max_num_seqs=conc + TBO on dp-attn cells at conc>=64 (was ATOM-default max_num_seqs + TBO per-scenario gate 256/1024) - add 1k1k scenario (was 8k1k only; mirrors prior grid) - 8k1k grid: align AMD dev sweep (run 20260723) + tp8 dp-off low-conc redundancy Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
max_req=conc is a mid/high-concurrency optimization. At very low conc (e.g. c2) the ATOM default is on-par or ~4% better, so only set --max-num-seqs=conc for conc>=64 (matches the TBO gate). High conc still gets it (removes the cap). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dp-attention keeps a full KV pool per rank, so the large default max_num_seqs OOMs even at low conc (c16/c32). Set max_req=conc for every dp-on cell; dp-off low conc still uses the ATOM default. TBO gate unchanged (conc>=64).
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the new tp4 dp-attn row (conc 16-128) overlapping the tp4 dp-off row (conc 1-64) contradicts the grid's own crossover comment — it doesn't: the same overlapping-ranges-to-recheck-the-crossover pattern is already used elsewhere in this file (e.g. dsv4-fp4-mi355x-sglang's tp4 dp-attn 16-128 / dp-off 1-32 rows), so this is consistent with existing sweep-design conventions, not a new bug.
Extended reasoning...
The main finding (perf-changelog entry prepended instead of appended) is a real hard-CI-failure per validate_perf_changelog.py, so I'm not approving this PR. I additionally looked into a candidate concern about the tp4 dp-attn/dp-off concurrency ranges overlapping in the new dsv4-fp4-mi355x-atom search-space grid, since it looked like it might contradict the inline crossover comment. Cross-referencing against the rest of configs/amd-master.yaml (e.g. the existing dsv4-fp4-mi355x-sglang entry at tp4 dp-attn 16-128 / dp-off 1-32), the same overlapping-range pattern is already an established convention for rechecking the dp crossover on a new image/config, so this is not an additional bug beyond what's already flagged inline.
| - config-keys: | ||
| - dsv4-fp4-mi355x-atom | ||
| description: | ||
| - "Re-sweep dsv4-fp4-mi355x-atom on latest atom-dev nightly (nightly_202607231538, was 202606161823). Recipe: max_num_seqs=conc + TBO on dp-attn cells at conc>=64 (was per-scenario gate 256/1024). 8k1k grid adds tp8 dp-off low-conc redundancy to recheck the crossover on the new image." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2325 | ||
|
|
There was a problem hiding this comment.
🔴 The new perf-changelog.yaml entry for dsv4-fp4-mi355x-atom is inserted at line 1 (top of file), but AGENTS.md mandates the changelog is append-only and read chronologically oldest-to-newest, with new entries appended to the end only. This isn't just a style nit — utils/validate_perf_changelog.py enforces byte-for-byte prefix immutability of the base file and will hard-fail CI in its current form; the entry needs to be moved to the end of perf-changelog.yaml.
Extended reasoning...
What's wrong: perf-changelog.yaml is documented in AGENTS.md (line 78) as being read chronologically — "oldest at top, newest at bottom" — with an explicit MUST: "new entries MUST be appended to the END, never inserted in the middle or prepended." Line 39 reiterates the file is "append-only; preserve whitespace." The PR's diff (@@ -1,3 +1,9 @@) instead inserts the new dsv4-fp4-mi355x-atom entry at line 1, pushing the previously-first entry (the 70b-fp8-*-vllm / PR #95 entry, one of the oldest entries in the file) down. I confirmed this against the file currently on disk: the new entry sits at lines 1-6, above #95, while much more recent entries (e.g. PR #2301) remain further down the file.
Why this isn't just a documentation nit: utils/validate_perf_changelog.py mechanically enforces this invariant against exactly this failure mode. validate_raw_change() (line ~211) does:
if not head_raw.startswith(base_raw):
raise ... "restore the base file byte-for-byte and append at the end"Since the head file's bytes now begin with the new entry instead of the base file's original first byte, head_raw.startswith(base_raw) is False, and this check fails. Separately, compare_entries() (line ~163) walks base_entries by index and diffs each against the corresponding head_entries[i]; since the new entry is now head_entries[0] while base_entries[0] is still the old 70b-fp8 entry, the comparison finds base_entries[0] != head_entries[0] (beyond an allowed pr-link-only correction) and raises "entry 1 changed; existing entries are immutable except for pr-link-only corrections." Either check alone is sufficient to hard-fail changelog validation in CI.
Step-by-step proof:
- Base
perf-changelog.yaml(main) starts with the70b-fp8-*-vllmentry (pr-link: .../pull/95) as its very first bytes. - This PR's diff adds 6 new lines at the very top (hunk
@@ -1,3 +1,9 @@), so the head file's first bytes are now the newdsv4-fp4-mi355x-atomentry, and the old70b-fp8entry has shifted down to start at what was line 1 in the base but is now offset by 6 lines in the head. validate_raw_change()computeshead_raw.startswith(base_raw)— comparing the head file's first N bytes (N = len(base_raw)) against the base file's bytes. Because the head file's first 6 lines are new content not present in the base at that offset, this isFalse.- The validator raises: "appended entries changed historical perf-changelog.yaml bytes; restore the base file byte-for-byte and append at the end." This is a hard failure, not a warning.
- Independently,
compare_entries()parses both files into entry lists and diffs by index;base_entries[0](the70b-fp8entry) no longer matcheshead_entries[0](now the newdsv4entry), raising a second, equally fatal error.
How to fix: Move the 6-line block (lines 1-6 in the diff) to the end of perf-changelog.yaml instead of the top — i.e., apply it as a pure append after the last existing entry, leaving every byte of the current file untouched and unshifted. This also matches the merge-conflict resolution recipe in AGENTS.md (line 100), which explicitly instructs re-appending a PR's own changelog entry "at the tail" when resolving perf-changelog.yaml conflicts, reinforcing that entries always belong at the end.
No refutations to address — all verifiers agreed on this finding, and I independently confirmed both the AGENTS.md rule and the exact validator code path (utils/validate_perf_changelog.py) that will fail CI as a result.
| - config-keys: | ||
| - dsv4-fp4-mi355x-atom | ||
| description: | ||
| - "Re-sweep dsv4-fp4-mi355x-atom on latest atom-dev nightly (nightly_202607231538, was 202606161823). Recipe: max_num_seqs=conc + TBO on dp-attn cells at conc>=64 (was per-scenario gate 256/1024). 8k1k grid adds tp8 dp-off low-conc redundancy to recheck the crossover on the new image." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2325 | ||
|
|
There was a problem hiding this comment.
🟡 AGENTS.md requires every PR title and description to be bilingual (English title + / <中文标题>, plus a mirrored ## 中文说明 body section), but this PR's title and body (What/Changes/Why/Note) are English-only with no Chinese translation anywhere.
Extended reasoning...
AGENTS.md line 7 states an explicit, mandatory contribution rule: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English." It specifies the required title format as <English title> / <中文标题>, and requires the body to follow the English content with a Chinese translation, e.g. a ## 中文说明 section mirroring the summary. The rule text is unambiguous that "this applies to every PR and every issue."
This PR's title is [AMD] dsv4-fp4-mi355x-atom: latest atom-dev image + opt recipe — no / <中文标题> suffix. The body contains ## What, ## Changes, ## Why the recipe change, and ## Note for reviewers sections, all in English, with no ## 中文说明 section or any Chinese text anywhere in the description. The perf-changelog.yaml entry added by this PR (lines 1-6) is the English-only changelog description that mirrors this English-only PR body, so the same gap is visible in the tracked file content, not just the PR metadata.
Nothing in the existing tooling (the recipe-reminder GitHub Action comment, CI checks, or the PR review checklist bot) enforces or checks for the bilingual title/body requirement — the automated reminder comment covers CODEOWNER review and the full-sweep-fail-fast label, not language compliance. So this rule is only enforced by human/AI reviewers reading AGENTS.md, and it was missed here.
Proof by inspection: comparing the PR's actual title/body against the AGENTS.md-mandated format —
- Required title format:
<English title> / <中文标题>. Actual title:[AMD] dsv4-fp4-mi355x-atom: latest atom-dev image + opt recipe— no/separator, no Chinese characters present. - Required body: English content followed by a
## 中文说明section mirroring it. Actual body sections:## What,## Changes,## Why the recipe change,## Note for reviewers— scanning all of them for any CJK characters turns up none. - Therefore the PR violates the literal, stated rule in AGENTS.md.
Fix is trivial and low-risk: append / <中文标题> to the PR title and add a ## 中文说明 section to the body mirroring the What/Changes/Why content in Simplified Chinese. This is a process/documentation compliance gap, not a functional defect — it doesn't affect the recipe script, the amd-master.yaml config, the benchmark sweep, or correctness of results, so it shouldn't block merge on its own, but it should be fixed to comply with the repo's contribution policy.
| - config-keys: | ||
| - dsv4-fp4-mi355x-atom | ||
| description: | ||
| - "Re-sweep dsv4-fp4-mi355x-atom on latest atom-dev nightly (nightly_202607231538, was 202606161823). Recipe: max_num_seqs=conc + TBO on dp-attn cells at conc>=64 (was per-scenario gate 256/1024). 8k1k grid adds tp8 dp-off low-conc redundancy to recheck the crossover on the new image." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2325 |
There was a problem hiding this comment.
🟡 The new dsv4-fp4-mi355x-atom changelog entry's pr-link points to #2325, but this entry is introduced by this PR (#2327) — #2325 was the prior dsv4-fp4-mi355x-atom PR. Every other entry in perf-changelog.yaml links to the PR that introduced it, so this should be updated to point to #2327.
Extended reasoning...
The new perf-changelog.yaml entry added by this PR sets pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2325, but this PR is #2327 per the PR metadata (title, description, and diff all describe the changes made in this PR: bumping the atom-dev image to nightly_202607231538, setting max-num-seqs=$CONC on dp-on cells, lowering the TBO gate to conc>=64, and reshaping the 8k1k grid). The changelog entry's description text matches these exact changes, confirming it was authored for and belongs to this PR.\n\nThe convention in perf-changelog.yaml, verified against numerous other entries in the file (e.g. pull/95, pull/110, pull/149), is that each entry's pr-link points to the PR that introduced that specific changelog entry — not to a related or historical PR for the same config-key. This entry breaks that convention by linking to #2325 instead.\n\nGit history explains how the stale value got there: commit bc81345 ("chore: fill pr-link for dsv4-fp4-mi355x-atom changelog (#2325)") shows that #2325 was itself an earlier, separate PR for this same dsv4-fp4-mi355x-atom config that filled in its own pr-link placeholder. When this new entry was added on top for the current re-sweep (PR #2327), the same PR number was seemingly copy-pasted from that prior commit rather than being updated to the current PR.\n\nProof: (1) PR metadata states this PR's number is 2327 and its description matches the new changelog entry's description word-for-word (image bump, max_num_seqs=conc, TBO gate at conc>=64, 8k1k grid changes). (2) The diff shows the changelog entry is wholly new (@@ -1,3 +1,9 @@ adds the full block, not an edit to a pre-existing entry). (3) Recent commit bc81345 in this same branch's history is literally titled "...changelog (#2325)", confirming 2325 refers to a distinct, earlier PR, not this one. (4) No other perf-changelog.yaml entry links to a PR other than the one that introduced it. Together this shows the link should read pull/2327, not pull/2325.\n\nImpact is limited to documentation/traceability: the pr-link field is metadata used to trace a changelog entry back to its introducing PR for future readers or tooling; nothing in the benchmark sweep, recipe script, or config execution reads or depends on this value, so nothing breaks functionally. The fix is a one-line change: update pr-link in perf-changelog.yaml from .../pull/2325 to .../pull/2327.
zufayu
changed the title
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30080242345 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30082074886 |
6 similar comments
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30082074886 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30082074886 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30082074886 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30082074886 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30082074886 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=30082074886 |
❌❌❌ REJECTED ❌❌❌@zufayu — this comment is not a CODEOWNER sign-off: it is a request for @chunfangamd to review, and it only quotes the checklist trigger phrase (which fired this gate). Blocking issues: no filled-in checklist, signer is not a CODEOWNER for ❌ Check 0 (CODEOWNER): FAIL — |
|
/stage-results 30082074886 |
|
@zufayu staged run 30082074886: https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-07-24~r30082074886 This shared staging slot remains available until the next |
|
Hi @chunfangamd — could you review this PR and post the filled-in PR_REVIEW_CHECKLIST from docs/PR_REVIEW_CHECKLIST.md? The full sweep is green — all TP4/TP8, dp-off/dp-on, throughput and evals passed:
Summary: re-sweep on After the checklist, an authorized maintainer needs to comment |
… appended after #2313) [skip-sweep]
|
Update: rebased on latest main and resolved the @chunfangamd @seungrokj @billishyahao — could one of you (CODEOWNER for The config/recipe is unchanged from the green sweep — all TP4/TP8, dp-off/dp-on, throughput + evals passed (gsm8k 0.964–0.967, above the 0.91 bar): Since the |
2 participants
What
Re-sweep
dsv4-fp4-mi355x-atom(DeepSeek-V4-Pro FP4, MI355X) on the latest atom-dev image with an optimized recipe.Changes
nightly_202606161823→nightly_202607231538(latest atom-dev)dsv4_fp4_mi355x_atom.sh):--max-num-seqs "$CONC"for all dp-on cells (mandatory: dp-attention keeps a full KV pool per rank, so the large defaultmax_num_seqsOOMs even at low conc like c16/c32) and forconc>=64. dp-off low conc keeps the ATOM default.conc>=64(was per-scenario gate 256/1024)Why the recipe change
AMD internal dev benchmarking shows this recipe beats the prior default:
max_num_seqs=concremoves the high-concurrency throughput cap and avoids dp-on OOMconc>=64from measured crossover (no measurable TBO gain below conc 64 on 8k/1k)Note for reviewers
Needs the
full-sweep-fail-fastlabel to run the sweep.RANDOM_RANGE_RATIOstays at the repo default (0.8), matching the dev runs.中文说明
内容
在最新 atom-dev 镜像上用优化配方重跑
dsv4-fp4-mi355x-atom(DeepSeek-V4-Pro FP4,MI355X)的 sweep。改动
nightly_202606161823→nightly_202607231538(最新 atom-dev)dsv4_fp4_mi355x_atom.sh):--max-num-seqs "$CONC"(必需:dp-attention 每 rank 保留完整 KV pool,默认的大 max_num_seqs 即使在 c16/c32 低并发也会 OOM),以及conc>=64。dp-off 低并发保持 ATOM 默认。conc>=64开启 TBO(原为按 scenario 门控 256/1024)配方改动原因
AMD 内部 dev benchmark 显示此配方优于原默认:
max_num_seqs=conc去掉高并发吞吐上限,并避免 dp-on OOMconc>=64(8k/1k 下 conc 64 以下无可测 TBO 收益)给 reviewer 的说明
需要
full-sweep-fail-fastlabel 才能跑 sweep。RANDOM_RANGE_RATIO保持 repo 默认(0.8),与 dev run 一致。Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com